[external-assets] Create external assets defs for undefined assets in repo construction #20145

smackesey · 2024-02-29T13:25:58Z

Summary & Motivation

Currently it is possible for assets to be referenced as dependencies when they have no corresponding AssetsDefinition (only when using deps). This complicates implementation of the AssetGraph and AssetLayer.

This PR generates unexecutable external assets during repository construction (the same place source assets are converted to external assets) for any referenced keys without a corresponding user-provided definition.

How I Tested These Changes

Existing test suite. One test was removed-- this made sure the internal asset graph could handle undefined asset references, but we no longer rely on this behavior.

smackesey · 2024-02-29T13:26:15Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @smackesey and the rest of your teammates on Graphite

schrockn · 2024-03-04T20:40:36Z

Does this cause any user-facing changes? Do we render bare asset key references differently?

schrockn

Req'ing changes for q mgmt

smackesey · 2024-03-04T21:22:12Z

Does this cause any user-facing changes? Do we render bare asset key references differently?

I didn't realize this but it turns out that we do. Given these defs with foo_source "bare" and bar_source a defined SourceAsset:

### 240304-stub_vs_source.py

from dagster import (
    asset,
    Definitions,
    AssetExecutionContext,
    SourceAsset,
)



@asset(deps=["foo_source"])
def foo(context: AssetExecutionContext):
    ...

bar_source = SourceAsset("bar_source")

@asset
def bar(context: AssetExecutionContext, bar_source):
    ...

defs = Definitions(
    assets=[foo, bar, bar_source],
)

Before this PR:

After this PR:

I'm not really sure why we do this since both foo_source and bar_source are AFAICT treated otherwise identically, but maybe we stick some special metadata on the auto-created stub defs to keep rendering the same?

schrockn · 2024-03-05T12:47:53Z

@smackesey on reflection I think this actually a fairly important product decision.

It gives us the opportunity to allow people to model external assets without even forcing them to explicitly learn the concept upfront. It will just appear in the UI. However we need to be deliberate about that decision as there are arguments both ways.

We should either

Do nothing
Add metadata to retain the old behavior
Propose this change to (esp Sandy and Josh) in an internal discussion

smackesey · 2024-03-05T14:00:00Z

So after some further investigation, the rendering change is smaller than I thought. The above difference I showed is only observed when looking at the default asset group. There is no change when looking at an asset job, which looks like this both before and after the change:

I believe the status quo style is probably not the result of a principled decision. Basically, the same graph rendering code is used for op-jobs, asset-jobs, and asset groups. The "small" style is used to show that a job is loading from source assets that aren't defined in the job, or an asset group has dependencies outside the group. The reason we see a difference when viewing the default group is that, in the status quo, the group_name of the ExternalAssetNode representing bare dependencies is null, whereas after this change it is default.

Further, I don't think the group name was ever intended to be set to null for bare dependencies, because we explicitly set it when defining the ExternalAssetNode for them-- it just happens to always be None because of the convoluted logic of `get_external_asset_nodes_from_defs:

    asset_keys_without_definitions = all_upstream_asset_keys.difference(assets_defs_by_key.keys())

    asset_nodes = [
        ExternalAssetNode(
            asset_key=asset_key,
            dependencies=list(deps[asset_key].values()),
            depended_by=list(dep_by[asset_key].values()),
            execution_type=AssetExecutionType.UNEXECUTABLE,
            job_names=[],
            group_name=group_name_by_asset_key.get(asset_key),  ## HERE
            code_version=code_version_by_asset_key.get(asset_key),
        )
        for asset_key in asset_keys_without_definitions
    ]

In any case, I updated the PR to keep the group_name of a bare dep as null until we make the above decision. I hope that allows us to proceed with this PR w/out being blocked on that decision. (Bare deps were already represented as ExternalAssetNode, the initially observed rendering change here was just from a slight change to the representation, i.e. having group name become "default").

FWIW I favor option (3), i.e. feel that bare deps should be treated just like explicitly defined SourceAsset. I had some discussion on it with @OwenKephart here: https://dagsterlabs.slack.com/archives/C03A0D72A6T/p1709238575807419

github-actions · 2024-03-05T14:30:20Z

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-nad859cn1-elementl.vercel.app
https://sean-external-assets-repo-create-assets-for-undefined.dagster.dagster-docs.io

Direct link to changed pages:

schrockn

Can you create linear task for eliminating SYSTEM_METADATA_KEY_AUTO_CREATED_STUB_ASSET and link to the issue in the places where we will want to delete when we come to a resolution on what to do about these bare references?

… repo construction

… repo construction (#20145) ## Summary & Motivation Currently it is possible for assets to be referenced as dependencies when they have no corresponding `AssetsDefinition` (only when using `deps`). This complicates implementation of the `AssetGraph` and `AssetLayer`. This PR generates unexecutable external assets during repository construction (the same place source assets are converted to external assets) for any referenced keys without a corresponding user-provided definition. ## How I Tested These Changes Existing test suite. One test was removed-- this made sure the internal asset graph could handle undefined asset references, but we no longer rely on this behavior.

smackesey mentioned this pull request Feb 29, 2024

[external-assets] Standardize terminology on "execution set" #20140

Merged

smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch 2 times, most recently from 212504b to 3efaa6f Compare February 29, 2024 17:03

smackesey force-pushed the sean/external-assets-execution-unit branch from 59f3d5f to 1fd1ee8 Compare February 29, 2024 17:21

smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from 3efaa6f to 5422a3b Compare February 29, 2024 17:21

smackesey force-pushed the sean/external-assets-execution-unit branch from 1fd1ee8 to b6ea604 Compare February 29, 2024 17:45

smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch 2 times, most recently from dce4ba8 to d77ea65 Compare February 29, 2024 18:16

smackesey changed the base branch from sean/external-assets-execution-unit to sean/external-assets-hoist-input-key-resolution-to-repo-builder March 1, 2024 16:36

smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from d77ea65 to 101a9c6 Compare March 1, 2024 16:36

This was referenced Mar 1, 2024

[external-assets] Hoist resolution of input asset keys to RepositoryDataBuilder #20186

Merged

[bugfix] Fix copying of metadata, description in assets_def.from_attributes #20189

Merged

smackesey marked this pull request as ready for review March 1, 2024 17:59

smackesey requested review from schrockn and OwenKephart March 1, 2024 17:59

smackesey force-pushed the sean/external-assets-hoist-input-key-resolution-to-repo-builder branch from 00eb64c to 2f03e42 Compare March 1, 2024 18:16

smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from 101a9c6 to 599d04b Compare March 1, 2024 18:16

smackesey force-pushed the sean/external-assets-hoist-input-key-resolution-to-repo-builder branch from 2f03e42 to 7fdcaea Compare March 1, 2024 18:47

smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from 599d04b to 311d0ef Compare March 1, 2024 18:47

smackesey force-pushed the sean/external-assets-hoist-input-key-resolution-to-repo-builder branch from 7fdcaea to ae90738 Compare March 1, 2024 19:31

smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from 311d0ef to 3bb4896 Compare March 1, 2024 19:31

smackesey force-pushed the sean/external-assets-hoist-input-key-resolution-to-repo-builder branch from ae90738 to 5003390 Compare March 2, 2024 20:43

smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from 3bb4896 to d73e923 Compare March 2, 2024 20:43

This was referenced Mar 3, 2024

[external-assets] Mark public RepositoryDefinition.{assets_defs_by_key, source_assets_by_key} #20221

Merged

[external-assets] Expose asset_checks_defs_by_key on RepositoryDefinition #20222

Merged

smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from d73e923 to c015d8f Compare March 3, 2024 21:40

smackesey mentioned this pull request Mar 3, 2024

Fix test for compatibility with pytest 8.1 #20225

Merged

smackesey force-pushed the sean/external-assets-hoist-input-key-resolution-to-repo-builder branch from 072ed7a to c2031ac Compare March 3, 2024 22:13

smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from c015d8f to 0b5aec4 Compare March 3, 2024 22:13

This was referenced Mar 3, 2024

[external-assets] Allow asset jobs to combine materializations and observations #19667

Merged

[external-assets] Build base asset jobs using AssetGraph #20227

Merged

smackesey force-pushed the sean/external-assets-hoist-input-key-resolution-to-repo-builder branch from c2031ac to c753e70 Compare March 4, 2024 16:56

smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from 0b5aec4 to cdd3edc Compare March 4, 2024 16:56

smackesey force-pushed the sean/external-assets-hoist-input-key-resolution-to-repo-builder branch from c753e70 to 9fa2ff0 Compare March 4, 2024 18:23

smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from cdd3edc to 94444b2 Compare March 4, 2024 18:23

smackesey force-pushed the sean/external-assets-hoist-input-key-resolution-to-repo-builder branch from 9fa2ff0 to 42c75ff Compare March 4, 2024 20:29

smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from 94444b2 to 98c621b Compare March 4, 2024 20:29

schrockn requested changes Mar 4, 2024

View reviewed changes

smackesey force-pushed the sean/external-assets-hoist-input-key-resolution-to-repo-builder branch from 42c75ff to 5401f3b Compare March 4, 2024 21:07

Base automatically changed from sean/external-assets-hoist-input-key-resolution-to-repo-builder to master March 4, 2024 21:25

smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch 2 times, most recently from edfd67f to d455262 Compare March 5, 2024 14:27

smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from d455262 to 5a2551a Compare March 5, 2024 14:35

schrockn approved these changes Mar 5, 2024

View reviewed changes

[external-assets] Create external assets defs for undefined assets in…

6eadaed

… repo construction

smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from 5a2551a to 6eadaed Compare March 5, 2024 18:12

smackesey merged commit db6db44 into master Mar 5, 2024
1 check was pending

smackesey deleted the sean/external-assets-repo-create-assets-for-undefined branch March 5, 2024 18:32

smackesey mentioned this pull request Mar 5, 2024

[external-assets] AssetGraph class renames #20276

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[external-assets] Create external assets defs for undefined assets in repo construction #20145

[external-assets] Create external assets defs for undefined assets in repo construction #20145

smackesey commented Feb 29, 2024 •

edited

Loading

smackesey commented Feb 29, 2024 •

edited

Loading

schrockn commented Mar 4, 2024

schrockn left a comment

smackesey commented Mar 4, 2024

schrockn commented Mar 5, 2024

smackesey commented Mar 5, 2024

github-actions bot commented Mar 5, 2024

schrockn left a comment

[external-assets] Create external assets defs for undefined assets in repo construction #20145

[external-assets] Create external assets defs for undefined assets in repo construction #20145

Conversation

smackesey commented Feb 29, 2024 • edited Loading

Summary & Motivation

How I Tested These Changes

smackesey commented Feb 29, 2024 • edited Loading

schrockn commented Mar 4, 2024

schrockn left a comment

Choose a reason for hiding this comment

smackesey commented Mar 4, 2024

schrockn commented Mar 5, 2024

smackesey commented Mar 5, 2024

github-actions bot commented Mar 5, 2024

schrockn left a comment

Choose a reason for hiding this comment

smackesey commented Feb 29, 2024 •

edited

Loading

smackesey commented Feb 29, 2024 •

edited

Loading